[xDS] A97 - JWT token file call creds#12242
Conversation
Zgoda91
force-pushed
the
A97_jwt_token_call_creds
branch
4 times, most recently
from
a8f76b0 to
c781460
Compare
|
@ejona86 Could you please review this PR when you get a chance? Thanks! |
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
The shape of what I saw looked good. I didn't pay attention to any of the tests yet. I need to look through the provider plumbing more. But wanted to send the comments I have.
alts/src/main/java/io/grpc/alts/JwtTokenFileCallCredentials.java
Outdated
Show resolved
Hide resolved
alts/src/testFixtures/java/io/grpc/alts/JwtTokenFileTestUtils.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/io/grpc/xds/internal/JwtTokenFileXdsCredentialsProvider.java
Outdated
Show resolved
Hide resolved
alts/src/main/java/io/grpc/alts/JwtTokenFileCallCredentials.java
Outdated
Show resolved
Hide resolved
alts/src/main/java/io/grpc/alts/JwtTokenFileCallCredentials.java
Outdated
Show resolved
Hide resolved
alts/src/main/java/io/grpc/alts/JwtTokenFileCallCredentials.java
Outdated
Show resolved
Hide resolved
Zgoda91
force-pushed
the
A97_jwt_token_call_creds
branch
2 times, most recently
from
85495b0 to
9f97bb5
Compare
|
@ejona86 PR ready for second round |
|
@ejona86 kindly reminder about this forgotten piece :) |
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
Just sending what I have.
We're going to want to make a clone of XdsCredentialsRegistry for CallCreds, because they are two different config namespaces, the gRFC says we'd create a new registry, and there's really no reason to provide both call creds and channel creds. However, I see that XdsCredentialsRegistry.getHardCodedClasses() should be deleted; we should just hard-code an empty list to InternalServiceProviders.loadAll(); none of this will be used on Android.
Zgoda91
force-pushed
the
A97_jwt_token_call_creds
branch
from
9f97bb5 to
3a36865
Compare
|
@ejona86 - I applied the fixes from your latest comments. I had to rebase the PR due to local compilation issues. However, all new changes can be found starting from commit 6: |
xds/src/main/java/io/grpc/xds/internal/JwtTokenFileXdsCredentialsProvider.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/io/grpc/xds/internal/JwtTokenFileXdsCredentialsProvider.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/io/grpc/xds/internal/JwtTokenFileXdsCredentialsProvider.java
Outdated
Show resolved
Hide resolved
|
@ejona86 - newest changes published |
| * configuration. | ||
| */ | ||
| @Internal | ||
| public abstract class XdsCallCredentialsProvider { |
There was a problem hiding this comment.
I'm not wild about io.grpc.xds.internal (JwtTokenFileCallCredentials) depending on io.grpc.xds (XdsCallCredentialsProvider) which depends on io.grpc.xds.internal (XdsCallCredentialsRegistry). Can we move this class to internal, or the JWT stuff to io.grpc.xds?
There was a problem hiding this comment.
Moved JWT classes to io.grpc.xds package
|
@Zgoda91, I fixed the Bazel build. You'll want to pull from your branch to pull in my commit before making further changes (or you'll need to merge later). |
2 participants
implementing gRFC A97 grpc/proposal#492